Skip to content

refactor!: Implement generics for CheckPoint, LocalChain, and spk_client types #1582

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

LagginTimes
Copy link
Contributor

@LagginTimes LagginTimes commented Aug 29, 2024

Implements #1937.

Description

This PR is a step towards header checkpointing for bdk_electrum. The goal is to be able to store whole headers in CheckPoint so they do not have to be re-downloaded. Storing headers this way would be a prerequisite for caching of merkle proofs and for median time passed.

Notes to the reviewers

Changelog notice

  • CheckPoint takes in a generic.
  • LocalChain and ChangeSet take in generics.
  • spk_client types can take in generics.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

@LagginTimes LagginTimes marked this pull request as draft August 29, 2024 09:59
@LagginTimes LagginTimes changed the title refactor(core): CheckPoint takes a generic refactor(core): Implement generics for CheckPoint and LocalChain Aug 29, 2024
@LagginTimes LagginTimes force-pushed the generic_checkpoint branch 3 times, most recently from a899049 to 7c13781 Compare September 2, 2024 02:53
@LagginTimes LagginTimes force-pushed the generic_checkpoint branch 2 times, most recently from ea1cf8b to bf90ea5 Compare September 3, 2024 06:15
@LagginTimes LagginTimes force-pushed the generic_checkpoint branch 5 times, most recently from c278804 to 6300d7c Compare September 5, 2024 03:43
@LagginTimes LagginTimes changed the title refactor(core): Implement generics for CheckPoint and LocalChain refactor(core): Implement generics for CheckPoint, LocalChain, and spk_client types Sep 5, 2024
@evanlinjin
Copy link
Member

@LagginTimes is this ready to review again? Remember to mark it as so and request reviewers, otherwise no one will know about it.

@LagginTimes LagginTimes force-pushed the generic_checkpoint branch 2 times, most recently from 3d7f48c to 69d8643 Compare May 14, 2025 19:22
@LagginTimes LagginTimes marked this pull request as ready for review May 14, 2025 20:02
@LagginTimes LagginTimes requested a review from evanlinjin May 14, 2025 20:02
@evanlinjin
Copy link
Member

What was the discussion around breaking changes? Were we okay to break the API?

@LagginTimes LagginTimes changed the title refactor: Implement generics for CheckPoint, LocalChain, and spk_client types refactor!: Implement generics for CheckPoint, LocalChain, and spk_client types May 15, 2025
@LagginTimes
Copy link
Contributor Author

What was the discussion around breaking changes? Were we okay to break the API?

Iirc the consensus was for this to be a breaking change to be a cleaner API.

Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pushing this forward.

Please double check (brownie points for triple check) over all the methods of a struct to see whether they are coherent, whether any more methods can be made generic and whether the method names and generic parameter names are coherent and make sense.

Let's make all generic parameter names for block data be consistent across the board. I'm in favor of B.

@LagginTimes LagginTimes force-pushed the generic_checkpoint branch from 585f35f to e894c73 Compare May 17, 2025 19:50
@LagginTimes LagginTimes marked this pull request as draft May 17, 2025 19:55
@LagginTimes LagginTimes force-pushed the generic_checkpoint branch 2 times, most recently from f135eb0 to 7699737 Compare May 19, 2025 19:47
@LagginTimes LagginTimes marked this pull request as ready for review May 19, 2025 19:48
@LagginTimes LagginTimes requested a review from evanlinjin May 19, 2025 19:48
@LagginTimes LagginTimes force-pushed the generic_checkpoint branch from 7699737 to 7d47e3e Compare May 21, 2025 18:51
@notmandatory notmandatory moved this from In Progress to Needs Review in BDK Chain May 22, 2025
@LagginTimes LagginTimes force-pushed the generic_checkpoint branch from 7d47e3e to b5d4872 Compare May 23, 2025 18:01
@ValuedMammal
Copy link
Collaborator

ValuedMammal commented Jun 8, 2025

Tests are passing for me.

But I think we should still have the function _check_changeset_is_applied. Edit: unless I misread an earlier comment saying it is no longer needed. I tested with and without it, plus the unit tests should be sufficient, so feel free to ignore.

diff
--- a/crates/chain/src/local_chain.rs
+++ b/crates/chain/src/local_chain.rs
@@ -270,8 +270,9 @@ where
         };
 
         let (mut chain, _) = Self::from_genesis(genesis_data);
         chain.apply_changeset(&changeset)?;
+        debug_assert!(chain._check_changeset_is_applied(&changeset));
         Ok(chain)
     }
 
     /// Construct a [`LocalChain`] from a given `checkpoint` tip.
@@ -303,16 +304,18 @@ where
         update: CheckPoint<D>,
     ) -> Result<ChangeSet<D>, CannotConnectError> {
         let (new_tip, changeset) = merge_chains(self.tip.clone(), update)?;
         self.tip = new_tip;
+        debug_assert!(self._check_changeset_is_applied(&changeset));
         Ok(changeset)
     }
 
     /// Apply the given `changeset`.
     pub fn apply_changeset(&mut self, changeset: &ChangeSet<D>) -> Result<(), MissingGenesisError> {
         let old_tip = self.tip.clone();
         let new_tip = apply_changeset_to_checkpoint(old_tip, changeset)?;
         self.tip = new_tip;
+        debug_assert!(self._check_changeset_is_applied(changeset));
         Ok(())
     }
 
     /// Derives an initial [`ChangeSet`], meaning that it can be applied to an empty chain to
@@ -401,8 +404,30 @@ where
             None => return Ok(ChangeSet::default()),
         };
         Ok(changeset)
     }
+
+    fn _check_changeset_is_applied(&self, changeset: &ChangeSet<D>) -> bool {
+        let mut cur = self.tip.clone();
+        for (&exp_height, exp_data) in changeset.blocks.iter().rev() {
+            match cur.get(exp_height) {
+                Some(cp) => {
+                    if cp.height() != exp_height
+                        || Some(cp.hash()) != exp_data.map(|d| d.to_blockhash())
+                    {
+                        return false;
+                    }
+                    cur = cp;
+                }
+                None => {
+                    if exp_data.is_some() {
+                        return false;
+                    }
+                }
+            }
+        }
+        true
+    }
 }

Copy link
Collaborator

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK e1e1959

/// Construct from an iterator of block data.
///
/// Returns `Err(None)` if `blocks` doesn't yield any data. If the blocks are not in ascending
/// height order, then returns the last [`CheckPoint`] that would have been extended.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// height order, then returns the last [`CheckPoint`] that would have been extended.
/// height order, then returns an `Err(..)` containing the last checkpoint that would have been extended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module-blockchain new feature New feature or request
Projects
Status: Needs Review
Development

Successfully merging this pull request may close these issues.

Implement generics for CheckPoint, LocalChain, and spk_client types
8 participants